Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix parquet benchmark schema #513

Merged
merged 1 commit into from
Jun 30, 2021
Merged

Fix parquet benchmark schema #513

merged 1 commit into from
Jun 30, 2021

Conversation

nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Jun 30, 2021

Which issue does this PR close?

None, I'm banking progress so I don't lose it.

Rationale for this change

The benchmark schema for nullable primitives and strings isn't the same as the non-null ones, leading to a slightly unfair comparison between the two.

What changes are included in this PR?

Changed the schemas.

Are there any user-facing changes?

No

We aren't comparing the right values
@github-actions github-actions bot added the parquet Changes to the parquet crate label Jun 30, 2021
@nevi-me
Copy link
Contributor Author

nevi-me commented Jun 30, 2021

CC @yordan-pavlov, I didn't pick this up in the original PR.

I don't think arrow can currently represent the schema that you had created for nullable primitive and string, so this caught my attention while working on a potential fix for the list reader.

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.09%. Comparing base (de62168) to head (3d6523a).
Report is 2894 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   82.64%   83.09%   +0.44%     
==========================================
  Files         165      165              
  Lines       45703    47110    +1407     
==========================================
+ Hits        37773    39146    +1373     
- Misses       7930     7964      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yordan-pavlov
Copy link
Contributor

good spot @nevi-me , thanks for fixing this; does that have any effect on the benchmark results?

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me

@nevi-me
Copy link
Contributor Author

nevi-me commented Jun 30, 2021

@yordan-pavlov I wasn't paying attention, but I'll check and post a comment here. I don't expect much of a change, because most of the work is done at the leaf, and that wouldn't change.

It's just that this benchmark stood out when there was a failure on my local branch while checking the perf impact of a change I was making.

@nevi-me nevi-me merged commit ef88876 into master Jun 30, 2021
@nevi-me nevi-me deleted the parquet-fix-list-reader branch June 30, 2021 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants